test(sca,workflows): pin routes, request bodies and wire tags - #69
Merged
Conversation
Close the SCA / workflows / models rows of the mutation ledger. sca: record route/body/params in mockHTTPClient; assert the exact route for every endpoint and the full POST body contents; replace the tautological TestListSessions_WithCSPFilter with an assertion on the sent csp query param; pin the on-demand target_category and pageSize literals; cover context propagation and the paginated decode errors; extract the X-API-Version guard into its own test; pin the sca/uar ISP service slugs via the constructed BaseURL. workflows: drive a 500 through all six checkResponse call sites; assert the exact finalize route and FinalizationReason; pin the limit query param and defaultPageSize; add logging_client_test.go (including the Authorization redaction guard) and service_config_test.go. models: pin the requestDetails, result, cancelReason and roleId request tags, and decode a populated accessCredentials off the wire through ParseAWSCredentials; assert the parsed values in TestParseAWSCredentials. No production code changed.
…rdicts Twelve PR8 rows named tests that the implementer consolidated away. Point each row at the test that actually exists, and say explicitly where several rows now share one test rather than repeating a name with no note. SCA-18 was recorded as OVERSTATED/refuted on a "not reproducible" verdict that came from a sandbox without loopback TCP. It has now been reproduced on both services (inbound requests = 4, want 1), so it flips to CONFIRMED/test naming the two retry-policy tests as its killers. SCA-15's identical clause is dropped: both X-API-Version mutants were executed and both fail. Record the reusable lesson in the preamble: a bare deletion of sdkclient.DisableTransientRetry orphans the import and is a compile kill, not a test kill; the honest mutation is the assignment form. Summary counts updated. Also: soften two test comments that claimed to replace assertions still present elsewhere, and assert the error message in the credentials "empty string" row so the previously inert empty-string guard is killed.
TestListRequests_SendsLimit already recorded mock.gotRoute and never asserted it, leaving /api/workflows/requests the only unpinned route in either service. TestSubmitRequest asserted only TargetCategory, so dropping RequestDetails at the service boundary survived the whole suite.
GroupsElevateTarget.groupId is the whole per-target payload of POST /api/access/elevate/groups, and GroupsElevateTargetResult.sessionId is the only handle for revoking a group session by ID. Five tags across the request and response models survived renaming.
The workflows models test pinned only the three request bodies while the SCA twin pinned both directions, leaving the keys `grant request get` and `grant request list` render free to be renamed. Form validation metadata remains unpinned and is recorded as a follow-up in the ledger.
The workflows mock returned the same *http.Response on every Get, so the second page of a paginating test would have read an already-drained Body. Replaced with a per-call queue that errors clearly when exhausted. TestServiceConfig_ReturnsIdsecServiceConfig asserted that ServiceConfig() returns the type its signature already declares; removed from both the workflows and SCA copies.
…ntract rows SCA-16 cited elevate.go:28 (the response model); the request model's roleId is at :6. WF-14 cited a test line for the cancel twin, against the preamble's rule that File:line is always the production site — named the test instead. Adds WF-22/WF-23 (list route, submit body), SCA-20..24 (group-elevation tags), WF-24 (workflows response tags, closed) and WF-25 (form validation metadata, recorded as a follow-up).
aaearon
force-pushed
the
test/wire-contracts
branch
from
August 16, 2026 07:42
8ca095a to
04fe58c
Compare
aaearon
added a commit
that referenced
this pull request
Aug 16, 2026
Squash-merging seven stacked PRs (#63-#69) that each carried their own copy of the ledger appended both the pre-work and post-work version of many rows. 48 IDs appeared twice inside the ## Ledger table. Collapse them, recompute the summary, and rewrite the rows whose production site no longer exists.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 8 of 8. Base:
test/isolation-harness(#63) — must merge first. Tests and docs only; no production change.The headline
Renaming the
accessCredentialsJSON tag oninternal/sca/models/elevate.gopassed the entire repo suite. It is the single fieldgrant envexists to deliver — rename it and every AWS elevation silently returns nil credentials. The existing tests only ever decoded"accessCredentials": null, and the service tests set the field to nil in Go structs the test itself marshalled, so the tag round-tripped trivially.Now decoded from a raw JSON literal off the wire through
ParseAWSCredentials, asserting all three values.Also
checkResponsecall sites ininternal/workflowscould be deleted undetected — the existing test only called the helper directly. A 500 decoded as an emptyAccessRequest, sogrant request submitprinted a blank request and exited 0. The 500 fixture is deliberately designed to decode cleanly into every response model, so removing a guard yields a genuine silent success rather than an incidental decode error.internal/workflows/logging_client.gohad no test file at all, including its Authorization-redaction path, while the byte-identical SCA one was fully covered. It is on the live path.TestListSessions_WithCSPFilterwas tautological — it asserted its own canned response.grant status --provider azurecould have been filtering nothing.FinalizeRequestcould drop the approver's reason and lose the request ID from the route; the test asserted onlyHasSuffix(route, "/finalize").X-API-Version: 2.0guard lived insideTestNewSCAAccessServiceDisablesTransientRetry, so a retry-motivated rename would have silently deleted it. Now its own named test.A won't-fix that became a fix
The ISP service slug was unpinned because both retry tests overwrite
client.BaseURLbefore issuing a request. I had allowed a documented won't-fix; the implementer pinned it instead, arguing the failure mode is total — every live request to the wrong host. Correct call.Recorded for the next reviewer
Two rows were previously marked "not reproducible" because a Codex sandbox blocked loopback TCP. Both reproduce fine here. The ledger preamble now records that a loopback-blocked sandbox cannot refute a row, and that a bare deletion of
DisableTransientRetryis a compile kill, not a test kill — the honest mutation is_ = sdkclient.DisableTransientRetry. That confusion cost two verification passes.29/30 mutations killed (the survivor pre-existing and inert). Adversarial review performed (Codex credits exhausted; review by a Claude agent). All findings fixed — ledger only; no Go file changed.